Skip to content

feat: add Description support for VariableDefinition (September 2025 spec)#1457

Open
asoorm wants to merge 4 commits intomasterfrom
ahmet/eng-9301-september-2025-spec-add-description-support-for
Open

feat: add Description support for VariableDefinition (September 2025 spec)#1457
asoorm wants to merge 4 commits intomasterfrom
ahmet/eng-9301-september-2025-spec-add-description-support-for

Conversation

@asoorm
Copy link
Copy Markdown
Contributor

@asoorm asoorm commented Mar 26, 2026

Implements Description? on VariableDefinition per the September 2025 GraphQL spec (graphql-spec #1170). Follows the same pattern as #1331 (OperationDefinition/FragmentDefinition).

query FindEmployee(
  "The unique employee identifier"
  $id: ID!
) {
  employee(id: $id) { ... }
}
  • Parser/printer: handle optional description before $Variable
  • JSON Schema builder: propagate variable description to property description, with fallback to schema argument description

Resolves ENG-9301

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 26, 2026

📝 Walkthrough

Walkthrough

Adds support for GraphQL variable definition descriptions: AST now stores descriptions, parser recognizes description tokens before variables, printer emits variable descriptions, JSON Schema builder sources descriptions from variables or corresponding argument definitions, and tests/fixtures updated.

Changes

Cohort / File(s) Summary
AST Structure
v2/pkg/ast/ast_variable_definition.go
Added optional Description field to VariableDefinition. Added Document.VariableDefinitionDescriptionBytes(ref int) and VariableDefinitionDescriptionString(ref int) helpers.
Parser Implementation
v2/pkg/astparser/parser.go
Extended parseVariableDefinitionList() to accept STRING/BLOCKSTRING as leading descriptions; added parseVariableDefinitionWithDescription(description *ast.Description) to attach parsed descriptions to variable nodes.
Parser Tests & Fixture
v2/pkg/astparser/parser_test.go, v2/pkg/astparser/testdata/variable_with_description.graphql
Added TestParseVariablesWithDescriptions and a fixture containing variables with single-line, block-string, and absent descriptions.
Printer Implementation
v2/pkg/astprinter/astprinter.go
In EnterVariableDefinition, emit variable description via PrintDescription() when Description.IsDefined, then print variable formatting.
Printer Tests
v2/pkg/astprinter/astprinter_test.go
Added test cases validating minified printing of variable definitions with single-line and block-string descriptions, mixed variable lists, and undescribed variables.
JSON Schema Generation
v2/pkg/engine/jsonschema/variables_schema.go
EnterVariableDefinition now sets schema description from variable description when present; added findArgumentDescriptionForVariable(varName string) to fallback to argument description from the operation/schema.
Schema Generation Tests
v2/pkg/engine/jsonschema/variables_schema_test.go
Added tests to TestBuildJsonSchema to verify description propagation from variable descriptions, from argument descriptions when variable lacks one, and omission when neither exists.
AST Importing
v2/pkg/astimport/astimport.go, v2/pkg/astimport/astimport_test.go
Importer now imports VariableDefinition.Description; added ImportDescription helper and a test TestImportVariableDefinitionWithDescription validating import of variable descriptions.

Sequence Diagram(s)

sequenceDiagram
    participant Source as GraphQL Source
    participant Parser as Parser
    participant AST as AST Document
    participant Printer as Printer
    participant Output as Formatted Output

    Source->>Parser: Raw GraphQL with variable descriptions
    Parser->>Parser: parseVariableDefinitionList()
    Parser->>Parser: detectDescription() → STRING / BLOCKSTRING
    Parser->>Parser: parseVariableDefinitionWithDescription()
    Parser->>AST: Create VariableDefinition with Description
    AST->>Printer: Walk AST (EnterVariableDefinition)
    Printer->>Printer: Check Description.IsDefined
    rect rgba(100, 200, 100, 0.5)
        Printer->>Printer: PrintDescription() if present
    end
    Printer->>Printer: Format variable name, type, default, directives
    Printer->>Output: Emit formatted variable definition
Loading
sequenceDiagram
    participant Ops as GraphQL Operation
    participant SchemaBuilder as Schema Builder
    participant Schema as GraphQL Schema
    participant JSONSchema as JSON Schema

    Ops->>SchemaBuilder: Process VariableDefinition
    SchemaBuilder->>SchemaBuilder: Check variable Description.IsDefined
    rect rgba(100, 150, 200, 0.5)
        alt Variable has description
            SchemaBuilder->>JSONSchema: Use variable description
        else Variable lacks description
            SchemaBuilder->>Ops: findArgumentDescriptionForVariable(varName)
            Ops->>SchemaBuilder: Return matched argument reference
            SchemaBuilder->>Schema: Resolve argument definition
            Schema-->>SchemaBuilder: Return argument description (if any)
            SchemaBuilder->>JSONSchema: Use argument description (if present)
        end
    end
    JSONSchema->>JSONSchema: Populate property description
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding Description support for VariableDefinition per the September 2025 GraphQL spec.
Description check ✅ Passed The description is related to the changeset, providing context about the GraphQL spec implementation, examples, and the fallback behavior for JSON schema.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ahmet/eng-9301-september-2025-spec-add-description-support-for

Comment @coderabbitai help to get the list of available commands and usage tips.

@asoorm asoorm force-pushed the ahmet/eng-9301-september-2025-spec-add-description-support-for branch 2 times, most recently from baf9ecb to 6b83257 Compare March 26, 2026 10:33
…spec)

Implements Description? on VariableDefinition per the September 2025
GraphQL spec (graphql-spec PR #1170), following the same pattern
established in PR #1331 for OperationDefinition and FragmentDefinition.

- Parser: handle optional string/block-string description before $Variable
- Printer: output variable descriptions when present
- JSON Schema Builder: propagate variable descriptions to JSON Schema
  property description fields, with fallback to argument descriptions
  from the schema definition when no variable description is provided

Resolves ENG-9301
@asoorm asoorm force-pushed the ahmet/eng-9301-september-2025-spec-add-description-support-for branch from 6b83257 to 5c581bb Compare March 26, 2026 12:06
@asoorm asoorm marked this pull request as ready for review March 27, 2026 12:18
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@asoorm
Copy link
Copy Markdown
Contributor Author

asoorm commented Mar 27, 2026

@claude review

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
v2/pkg/astparser/parser.go (1)

1560-1570: Minor duplication in list.Refs initialization can be collapsed.

Both branches initialize list.Refs twice around parse calls. A small helper/guard would reduce noise and keep both paths consistent.

Also applies to: 1572-1579

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astparser/parser.go` around lines 1560 - 1570, The duplicated
initialization of list.Refs around the parse calls should be collapsed: ensure
list.Refs is initialized once before calling p.parseDescription() /
p.parseVariableDefinitionWithDescription by extracting the guard into a small
helper or moving the cap(list.Refs) check (using
p.document.Refs[p.document.NextRefIndex()][:0]) to a single pre-check. Update
the branches that handle keyword.STRING/keyword.BLOCKSTRING (and the similar
block at the other occurrence) to call that single-init helper or perform the
one-time initialization before invoking p.parseDescription() and
p.parseVariableDefinitionWithDescription so list.Refs is only set once.
v2/pkg/astprinter/astprinter_test.go (1)

790-819: Consider adding one runIndent case for variable descriptions.

These new cases validate compact output well; adding an indented-mode assertion would harden formatting behavior for multiline/block descriptions too.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astprinter/astprinter_test.go` around lines 790 - 819, Add a
complementary indented-mode test by calling runIndent in the same test block
suite (e.g. alongside the "variable with block string description" case) to
assert indented formatting for variable descriptions; use the same input used in
that case and provide the expected pretty-printed, multi-line indented output as
the second argument to runIndent so the test verifies block-string and multiline
variable descriptions in indented mode (refer to the existing run and runIndent
test helpers and the "variable with block string description" test name to
locate where to add it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@v2/pkg/ast/ast_variable_definition.go`:
- Line 21: The import path that reconstructs VariableDefinition instances omits
the new Description field, causing metadata loss; update the reconstruction
logic in the import code (the block in astimport.go that builds
VariableDefinition objects around the 162-180 area) to copy the source
Description into the target VariableDefinition.Description when
creating/importing variables, and ensure any helper like ImportVariable or
ImportOperation propagation also preserves Description; add/update a unit test
that imports an operation with a variable Description and asserts the imported
VariableDefinition.Description is preserved.

In `@v2/pkg/engine/jsonschema/variables_schema.go`:
- Around line 194-238: The current fallback only checks root selection-set
fields and direct ValueKindVariable arguments, missing variables in nested
fields, inline fragments, fragment spreads, directives, and wrapped values;
update the logic that starts at operationDef.SelectionSet to recursively
traverse the full selection tree (follow SelectionSetRefs, resolve
FragmentSpread refs via v.operationDocument.FragmentDefinitions, handle
InlineFragment selections, and descend into nested field selections) and when
inspecting arguments use ArgumentValue to recursively unwrap List/Object values
to find any nested Variable kinds (using VariableValueNameString); for each
located variable, resolve its argument definition by locating the exact field's
definition based on the parent type (not just rootType.Ref) via
v.definitionDocument.ObjectTypeDefinitions and
FieldDefinitionArgumentsDefinitions and return
InputValueDefinitionDescriptionString when found.

---

Nitpick comments:
In `@v2/pkg/astparser/parser.go`:
- Around line 1560-1570: The duplicated initialization of list.Refs around the
parse calls should be collapsed: ensure list.Refs is initialized once before
calling p.parseDescription() / p.parseVariableDefinitionWithDescription by
extracting the guard into a small helper or moving the cap(list.Refs) check
(using p.document.Refs[p.document.NextRefIndex()][:0]) to a single pre-check.
Update the branches that handle keyword.STRING/keyword.BLOCKSTRING (and the
similar block at the other occurrence) to call that single-init helper or
perform the one-time initialization before invoking p.parseDescription() and
p.parseVariableDefinitionWithDescription so list.Refs is only set once.

In `@v2/pkg/astprinter/astprinter_test.go`:
- Around line 790-819: Add a complementary indented-mode test by calling
runIndent in the same test block suite (e.g. alongside the "variable with block
string description" case) to assert indented formatting for variable
descriptions; use the same input used in that case and provide the expected
pretty-printed, multi-line indented output as the second argument to runIndent
so the test verifies block-string and multiline variable descriptions in
indented mode (refer to the existing run and runIndent test helpers and the
"variable with block string description" test name to locate where to add it).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 63defb37-2976-4475-892f-7d83e80b0ea1

📥 Commits

Reviewing files that changed from the base of the PR and between f3974f6 and 1baf038.

📒 Files selected for processing (8)
  • v2/pkg/ast/ast_variable_definition.go
  • v2/pkg/astparser/parser.go
  • v2/pkg/astparser/parser_test.go
  • v2/pkg/astparser/testdata/variable_with_description.graphql
  • v2/pkg/astprinter/astprinter.go
  • v2/pkg/astprinter/astprinter_test.go
  • v2/pkg/engine/jsonschema/variables_schema.go
  • v2/pkg/engine/jsonschema/variables_schema_test.go

Comment thread v2/pkg/ast/ast_variable_definition.go
Comment on lines +194 to +238
// Iterate through root fields in the operation's selection set
selectionSetRef := operationDef.SelectionSet
for _, selectionRef := range v.operationDocument.SelectionSets[selectionSetRef].SelectionRefs {
selection := v.operationDocument.Selections[selectionRef]
if selection.Kind != ast.SelectionKindField {
continue
}

fieldRef := selection.Ref
// Check if this field has arguments referencing our variable
if !v.operationDocument.FieldHasArguments(fieldRef) {
continue
}

for _, argRef := range v.operationDocument.Fields[fieldRef].Arguments.Refs {
argValue := v.operationDocument.ArgumentValue(argRef)
if argValue.Kind != ast.ValueKindVariable {
continue
}
argVarName := v.operationDocument.VariableValueNameString(argValue.Ref)
if argVarName != varName {
continue
}

// Found the argument that references this variable.
// Look up the corresponding argument definition in the schema.
argName := v.operationDocument.ArgumentNameString(argRef)
fieldName := v.operationDocument.FieldNameString(fieldRef)

// Find this field in the root type definition
for _, fieldDefRef := range v.definitionDocument.ObjectTypeDefinitions[rootType.Ref].FieldsDefinition.Refs {
if v.definitionDocument.FieldDefinitionNameString(fieldDefRef) != fieldName {
continue
}
// Find the argument definition
for _, argDefRef := range v.definitionDocument.FieldDefinitionArgumentsDefinitions(fieldDefRef) {
if v.definitionDocument.InputValueDefinitionNameString(argDefRef) == argName {
if v.definitionDocument.InputValueDefinitions[argDefRef].Description.IsDefined {
return v.definitionDocument.InputValueDefinitionDescriptionString(argDefRef)
}
}
}
}
}
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fallback lookup misses valid variable usages outside root field arguments.

At Line 194, lookup only scans root selection-set fields and only direct argument values (ValueKindVariable). Variables used in nested fields, fragment spreads/inline fragments, directives, or wrapped structures won’t get fallback descriptions, so AC3 is only partially satisfied.

Please expand traversal to cover the full operation selection tree (including fragments) and resolve argument definitions in that context.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/engine/jsonschema/variables_schema.go` around lines 194 - 238, The
current fallback only checks root selection-set fields and direct
ValueKindVariable arguments, missing variables in nested fields, inline
fragments, fragment spreads, directives, and wrapped values; update the logic
that starts at operationDef.SelectionSet to recursively traverse the full
selection tree (follow SelectionSetRefs, resolve FragmentSpread refs via
v.operationDocument.FragmentDefinitions, handle InlineFragment selections, and
descend into nested field selections) and when inspecting arguments use
ArgumentValue to recursively unwrap List/Object values to find any nested
Variable kinds (using VariableValueNameString); for each located variable,
resolve its argument definition by locating the exact field's definition based
on the parent type (not just rootType.Ref) via
v.definitionDocument.ObjectTypeDefinitions and
FieldDefinitionArgumentsDefinitions and return
InputValueDefinitionDescriptionString when found.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially it is right, in case you are not normalizing operation before applying this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

Good point — let me check whether normalization is always applied to the operation before this builder runs.


🏁 Script executed:

#!/bin/bash
# Find all call sites of BuildJsonSchema and NewVariablesSchemaBuilder
rg -n "BuildJsonSchema\|NewVariablesSchemaBuilder\|NewVariablesSchemaBuilderWithOptions" --type go -C 5

Length of output: 0


🏁 Script executed:

#!/bin/bash
rg -n -e "BuildJsonSchema" -e "NewVariablesSchemaBuilder" --type go -C 6

Length of output: 29903


🏁 Script executed:

#!/bin/bash
# Search for external callers of the jsonschema package
rg -n "jsonschema\." --type go -g '!*variables_schema*' -C 4

# Also look for normalization happening before schema building in the engine
rg -n "Normalize\|astnormalization" --type go -C 3 | head -80

Length of output: 1801


🏁 Script executed:

#!/bin/bash
# Find files importing the engine/jsonschema package
rg -n "engine/jsonschema" --type go -C 3

# Also check where normalization is applied relative to JSON schema building
rg -n "NormalizeOperation\|NormalizeNamedOperation\|astnormalization\.NewNormalizer" --type go -C 3 | head -100

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check if variables_schema.go is a new file (git status)
git log --oneline -5 -- v2/pkg/engine/jsonschema/variables_schema.go

# Search for Normalize usage
rg -n "Normalize" --type go -l | head -20

# Check if there's a normalization step documented in the package or any README
fd "README" --type f | head -10

Length of output: 1175


🏁 Script executed:

#!/bin/bash
# Check how the jsonschema package is referenced across the repo
rg -rn "variables_schema\|VariablesSchema\|BuildJson" --type go -l

# Look at the MCP-related code to see if normalization happens before BuildJsonSchema
rg -rn "BuildJsonSchema\|VariablesSchemaBuilder" --type go

# Check normalization in planner.go context
rg -n "Normalize" v2/pkg/engine/plan/planner.go -C 3 | head -50

Length of output: 184


@devsergiy — that's a sharp observation. From inspecting the codebase, BuildJsonSchema/VariablesSchemaBuilder currently have no external callers outside their own test file, and those tests call it on a raw parsed (non-normalized) ast.Document. So the contract of "normalized vs. not" is currently undefined.

You're right that if normalization is guaranteed to run first, the limitation largely goes away — fragment spreads would be inlined, nested selections would be flattened to root fields, and the root-selection-set scan in findArgumentDescriptionForVariable would be sufficient for all practical cases.

However, a few nuances are worth noting regardless of normalization:

  1. AC2 (variable's own description) is unaffected — it reads directly from VariableDefinitions[ref].Description, so it works correctly whether or not the operation is normalized.

  2. AC3 (schema argument description fallback) is the impacted path — if an operation is not normalized before this builder runs, variables in nested fields or referenced via fragment spreads won't receive the fallback description from the schema. This is the scenario the original comment flagged.

  3. Normalization doesn't fully eliminate the concern — even after normalization, a variable passed only to a nested (non-root) field argument wouldn't be matched by the current root-only scan, since normalization inlines fragments but doesn't move all selections to depth-1.

Would it make sense to document the normalization precondition explicitly (e.g., in the BuildJsonSchema godoc), so callers know what to expect for AC3? Alternatively, if this is always expected to be called post-normalization in practice, the original comment can be treated as a known, acceptable limitation scoped to best-effort fallback behaviour.


🧠 Learnings used
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1456
File: v2/pkg/engine/plan/cost.go:657-672
Timestamp: 2026-03-24T09:11:24.567Z
Learning: In wundergraph/graphql-go-tools (v2/pkg/engine/plan/cost.go), the execution engine converts all inline literal argument values into variables before cost validation runs. Therefore, in `validateSliceArguments`, it is correct to only check `arg.hasVariable` when counting satisfied slicing arguments — there is no need to additionally handle `arg.intValue > 0` for literal values. Suggesting such a change is a false positive for this codebase.

Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).

Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1456
File: v2/pkg/engine/plan/cost.go:245-255
Timestamp: 2026-03-24T08:45:07.824Z
Learning: In wundergraph/graphql-go-tools (v2/pkg/engine/plan/cost.go), when validating `RequireOneSlicingArgument` for abstract fields, the intended behavior is: if ANY implementing concrete type has `RequireOneSlicingArgument=true`, the operation must satisfy the slicing argument constraint. The `requiringOneArgImplementingField` method intentionally returns the first matching implementing type's `FieldListSize` to trigger this strict validation. An operation cannot be considered valid for an abstract field if any concrete resolution would fail the slicing argument requirement.

Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1363
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:223-238
Timestamp: 2026-01-22T13:32:05.068Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go, ensure that inline fragments do not generate dedicated messages. Treat inline fragments as meta information about potentially selectable types (Oneof in protobuf). When EnterSelectionSet visits a field with an interface/union type, apply meta information directly and do not traverse a second selection for the inline fragment. Inline fragments should be handled via the parent field’s message (ResolveInlineFragment path) and added to FieldSelectionSet in EnterField; nested structure is managed by the parent, not by separate fragment messages.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you also add a test with a single line operation without line breaks?

Comment thread v2/pkg/astparser/parser.go Outdated
list.Refs = p.document.Refs[p.document.NextRefIndex()][:0]
}
ref := p.parseVariableDefinitionWithDescription(&description)
if cap(list.Refs) == 0 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to do it twice

Comment thread v2/pkg/astprinter/astprinter_test.go Outdated
"""
fragment UserFields on User {id name}`)
})
t.Run("variable with single-line description", func(t *testing.T) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you scope it to t.Run("variables descriptions")

Comment on lines +194 to +238
// Iterate through root fields in the operation's selection set
selectionSetRef := operationDef.SelectionSet
for _, selectionRef := range v.operationDocument.SelectionSets[selectionSetRef].SelectionRefs {
selection := v.operationDocument.Selections[selectionRef]
if selection.Kind != ast.SelectionKindField {
continue
}

fieldRef := selection.Ref
// Check if this field has arguments referencing our variable
if !v.operationDocument.FieldHasArguments(fieldRef) {
continue
}

for _, argRef := range v.operationDocument.Fields[fieldRef].Arguments.Refs {
argValue := v.operationDocument.ArgumentValue(argRef)
if argValue.Kind != ast.ValueKindVariable {
continue
}
argVarName := v.operationDocument.VariableValueNameString(argValue.Ref)
if argVarName != varName {
continue
}

// Found the argument that references this variable.
// Look up the corresponding argument definition in the schema.
argName := v.operationDocument.ArgumentNameString(argRef)
fieldName := v.operationDocument.FieldNameString(fieldRef)

// Find this field in the root type definition
for _, fieldDefRef := range v.definitionDocument.ObjectTypeDefinitions[rootType.Ref].FieldsDefinition.Refs {
if v.definitionDocument.FieldDefinitionNameString(fieldDefRef) != fieldName {
continue
}
// Find the argument definition
for _, argDefRef := range v.definitionDocument.FieldDefinitionArgumentsDefinitions(fieldDefRef) {
if v.definitionDocument.InputValueDefinitionNameString(argDefRef) == argName {
if v.definitionDocument.InputValueDefinitions[argDefRef].Description.IsDefined {
return v.definitionDocument.InputValueDefinitionDescriptionString(argDefRef)
}
}
}
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially it is right, in case you are not normalizing operation before applying this

assert.JSONEq(t, expectedJSON, string(data), "JSON schema does not match expected structure")
})

t.Run("variable with description propagated to JSON schema", func(t *testing.T) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per coderabbit comment it makes sense to add a test for that

an example
https://graphql.org/learn/queries/#using-variables-inside-fragments

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, reopen this pull request to trigger a review.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
v2/pkg/astimport/astimport.go (1)

165-210: ⚠️ Potential issue | 🟡 Minor

Preserve Description.IsBlockString when importing.

ImportDescription rebuilds via to.ImportDescription(descStr), which infers block-string mode from content. This can flip a single-line block-string description into a normal string during import.

💡 Proposed fix
 func (i *Importer) ImportDescription(description ast.Description, from, to *ast.Document) ast.Description {
 	if !description.IsDefined {
 		return ast.Description{}
 	}
-	descStr := from.Input.ByteSliceString(description.Content)
-	return to.ImportDescription(descStr)
+	return ast.Description{
+		IsDefined:     true,
+		IsBlockString: description.IsBlockString,
+		Content:       to.Input.AppendInputBytes(from.Input.ByteSlice(description.Content)),
+	}
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astimport/astimport.go` around lines 165 - 210, ImportDescription
currently rebuilds the description via to.ImportDescription(descStr) which can
flip Description.IsBlockString; change Importer.ImportDescription so after
obtaining descStr := from.Input.ByteSliceString(description.Content) and calling
desc := to.ImportDescription(descStr) you explicitly set desc.IsBlockString =
description.IsBlockString (and preserve description.IsDefined) before returning
desc, ensuring the original IsBlockString flag is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@v2/pkg/astimport/astimport.go`:
- Around line 165-210: ImportDescription currently rebuilds the description via
to.ImportDescription(descStr) which can flip Description.IsBlockString; change
Importer.ImportDescription so after obtaining descStr :=
from.Input.ByteSliceString(description.Content) and calling desc :=
to.ImportDescription(descStr) you explicitly set desc.IsBlockString =
description.IsBlockString (and preserve description.IsDefined) before returning
desc, ensuring the original IsBlockString flag is preserved.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a941a9c5-1676-45f8-bbf7-10572d31e095

📥 Commits

Reviewing files that changed from the base of the PR and between 1baf038 and a3d6b94.

📒 Files selected for processing (6)
  • v2/pkg/astimport/astimport.go
  • v2/pkg/astimport/astimport_test.go
  • v2/pkg/astparser/parser.go
  • v2/pkg/astparser/parser_test.go
  • v2/pkg/astparser/testdata/variable_with_description.graphql
  • v2/pkg/astprinter/astprinter_test.go
✅ Files skipped from review due to trivial changes (1)
  • v2/pkg/astparser/testdata/variable_with_description.graphql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants